Skip to content

fix: ComfyUI timeout/resume + websocket wait + multi-server + music tool - #475

Merged
calesthio merged 7 commits into
calesthio:mainfrom
nbsumbana-pixel:fix/comfyui-video-timeout-resume
Aug 13, 2026
Merged

fix: ComfyUI timeout/resume + websocket wait + multi-server + music tool#475
calesthio merged 7 commits into
calesthio:mainfrom
nbsumbana-pixel:fix/comfyui-video-timeout-resume

Conversation

@nbsumbana-pixel

@nbsumbana-pixel nbsumbana-pixel commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Summary

Closes out all four open questions in docs/comfyui-adapter-plan.md:

  • Timeout/resume: raise the default ComfyUI video generation timeout from 900s to 3600s (configurable via timeout_seconds) — non-accelerated local-GPU workflows (e.g. Wan 1.3B at 832x480/81-97 frames) were observed taking ~1360-1630s and false-failing under the old default even though the job was still completing server-side. ComfyUIError now carries prompt_id on both execution errors and timeouts; resume_prompt_id lets a timed-out-but-still-running job be resumed instead of resubmitted.
  • Websocket wait: generate() now waits via ComfyUI's websocket feed by default (wait_ws()), reacting to executing/execution_error events immediately instead of sleeping between REST polls. Optional on_progress callback; comfyui_video/comfyui_music print step progress on long renders. websocket-client is an optional import — _wait() falls back to REST poll() (with the remaining time budget, not a fresh one) when unavailable.
  • Multi-server: ComfyUIClient(capability="image"|"video"|"music") resolves its server URL from a per-capability env var (COMFYUI_IMAGE_SERVER_URL / COMFYUI_VIDEO_SERVER_URL / COMFYUI_MUSIC_SERVER_URL) first, then the shared COMFYUI_SERVER_URL, then the localhost default — so each capability can point at a different ComfyUI instance with zero extra config for single-server setups.
  • comfyui_music (new tool): ships with a bundled ACE-Step v1 (3.5B) text-to-audio workflow. The node-pack fragmentation that originally blocked this turned out to be moot for v1 — ComfyUI ships TextEncodeAceStepAudio/EmptyAceStepLatentAudio as native core nodes, not a third-party pack. tools/_comfyui/workflows/ace-step-1-t2a.json was built by cross-checking every node's class_type/inputs against ComfyUI's own source (nodes_ace.py, nodes_audio.py, nodes_latent.py, nodes.py), not trusting the UI-format export directly. prompt maps to ACE-Step's tags field, lyrics/duration_seconds/steps/cfg/lyrics_strength/seed are all patchable, and workflow_json/workflow_path + output_node remains available for ACE-Step 1.5 or other node packs. Routed through the existing registry.get_by_capability("music_generation") path alongside suno_music/music_gen. generate() now also reads the "audio" output key (what SaveAudioMP3/SaveAudio write). Gets timeout/resume/websocket-wait/multi-server for free via the shared client.

Also updates docs/comfyui-adapter-plan.md and .agents/skills/comfyui/SKILL.md throughout.

Test plan

  • pytest tests/contracts/test_comfyui_tools.py — 117 passed, covering timeout/resume, websocket completion/error/timeout/fallback paths, multi-server priority/isolation, and the full comfyui_music contract (bundled-path generation/missing-models/status plus custom-workflow override, shared TestContract suite, and a workflow-JSON validity check).
  • pytest tests/ -k "comfy or registry or selector or music" — 183 passed, 1 skipped.

🤖 Generated with Claude Code

Non-accelerated local-GPU workflows (e.g. Wan 1.3B at 832x480/81-97
frames) routinely took ~1360-1630s, so the old 900s default false-failed
real renders that were still completing server-side. Timeout is now a
configurable timeout_seconds input (default 3600s), and ComfyUIError
carries the prompt_id on error/timeout so a timed-out-but-still-running
job can be resumed via resume_prompt_id instead of resubmitted.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Resolves the "async generation" open question from the adapter plan.
generate() now watches ComfyUI's websocket events (executing/progress/
execution_error) and reacts immediately instead of sleeping between REST
polls, with an optional on_progress callback that comfyui_video uses to
print step progress on long renders. websocket-client is an optional
import; _wait() falls back to the original poll() loop (with the
remaining time budget, not a fresh one) when it's unavailable or the
connection drops, so resume_prompt_id recovery is unaffected either way.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@nbsumbana-pixel nbsumbana-pixel changed the title fix: raise ComfyUI video timeout default and add job resume support fix: ComfyUI video timeout/resume + websocket-based wait Aug 6, 2026
Resolves the "multi-server" open question from the adapter plan.
ComfyUIClient(capability="image"|"video") now resolves its server URL
from COMFYUI_IMAGE_SERVER_URL / COMFYUI_VIDEO_SERVER_URL first, falling
back to the shared COMFYUI_SERVER_URL and then the localhost default —
so comfyui_image and comfyui_video can point at separate ComfyUI
instances (different GPUs, different model sets) with zero extra config
for single-server setups. is_default_url/unavailable_reason() and the
setup_offer metadata account for the override.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@nbsumbana-pixel nbsumbana-pixel changed the title fix: ComfyUI video timeout/resume + websocket-based wait fix: ComfyUI video timeout/resume + websocket wait + multi-server Aug 6, 2026
Resolves the "music generation" open question from the adapter plan.
Unlike comfyui_image/comfyui_video there is no bundled workflow: ACE-Step's
ComfyUI node interface isn't standardized across custom node packs
(AceStepModelLoader vs native TextEncodeAceStepAudio, etc.), so instead of
picking one pack and breaking for everyone else, comfyui_music always
requires a caller-supplied workflow_json/workflow_path + output_node --
the same override contract image/video offer as an alternative, just
mandatory here. prompt is provenance-only, never injected into the graph.

Routed through the existing registry.get_by_capability("music_generation")
path alongside suno_music/music_gen -- no dedicated selector needed.
ComfyUIClient.generate() now also reads the "audio" output key (what
ComfyUI's native SaveAudio node writes), and gets timeout/resume/websocket-
wait/multi-server support for free via the shared client. Duration is a
best-effort ffprobe probe of the downloaded file since a custom workflow
gives no other way to know it ahead of time.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@nbsumbana-pixel nbsumbana-pixel changed the title fix: ComfyUI video timeout/resume + websocket wait + multi-server fix: ComfyUI timeout/resume + websocket wait + multi-server + music tool Aug 6, 2026
ACE-Step v1's node-pack fragmentation turns out to be moot: ComfyUI ships
TextEncodeAceStepAudio/EmptyAceStepLatentAudio as native core nodes
(comfy_extras/nodes_ace.py), not a third-party pack, and Comfy-Org's own
workflow_templates repo has an official ACE-Step-v1 template built from
those plus long-stable core nodes. tools/_comfyui/workflows/ace-step-1-t2a.json
was built by cross-checking every node's class_type and input names against
ComfyUI's own source (nodes_ace.py, nodes_audio.py, nodes_latent.py,
nodes.py) rather than trusting the UI-format export directly.

comfyui_music now defaults to this bundled workflow: prompt maps to
ACE-Step's tags field (matching suno_music's "prompt = music description"
convention), lyrics/duration_seconds/steps/cfg/lyrics_strength/seed are all
patchable, and missing ace_step_v1_3.5b.safetensors surfaces through the
same missing_models contract as image/video. workflow_json/workflow_path +
output_node remains available for ACE-Step 1.5, other node packs, or
different audio models entirely.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

@calesthio calesthio left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for closing the ComfyUI timeout/resume, multi-server, and local-music gaps. The focused contract suite is strong (117 passed), the timeout carries prompt_id, REST fallback keeps the remaining deadline, model readiness is structured, and the ACE-Step workflow is carefully documented.

I found a merge-blocking race in the new default websocket path:

  • generate() submits the workflow first and only then opens /ws?clientId=.... ComfyUI websocket events are not replayed. A fast job can finish between submit() and create_connection(), leaving wait_ws() waiting until the full timeout even though /history/{prompt_id} already contains the result.
  • Resume is also unsafe across a new process/tool instance: resume_prompt_id belongs to the original client_id, but a new ComfyUIClient creates a new UUID. Completion events for the original job are not guaranteed to be delivered to that new websocket. Because a websocket deadline raises ComfyUIError, _wait() explicitly refuses the REST fallback and can time out on a completed resumable job.

Please make completion history authoritative: check /history/{prompt_id} before waiting, poll it during websocket idle periods and/or use REST polling for resume IDs whose original client ID is unavailable. Add tests for (1) a job already completed before websocket connection and (2) resuming from a fresh client instance with websocket-client installed.

One additional correctness fix is needed in the new music tool: seed = inputs.get("seed") or random_seed() replaces an explicit valid seed of 0 with a random value, while idempotency records seed 0. Use an is None check and add a seed-zero regression test.

The branch is also behind current main; please update it after these fixes. Thanks?this is a valuable and well-tested direction once the websocket race and deterministic-seed contract are closed.

@calesthio calesthio left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the ComfyUI timeout/resume and music work. History is authoritative across websocket races and fresh-client resumes, seed zero is deterministic, and final CI passes. This is merge-ready.

@calesthio
calesthio merged commit 6e957f2 into calesthio:main Aug 13, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants